-
Notifications
You must be signed in to change notification settings - Fork 933
Handle OAuth Token Refreshes Using Background Thread For Admin, Producer and Consumer Clients #2130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
MSeal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wanted to check on expected timeout here and where we picked the value first, but otherwise lgtm
| if (!h->oauth_cb) | ||
| return 0; | ||
|
|
||
| int max_wait_sec = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did we pick this timeout? I'm not sure what the longest oauth wait time will be. Maybe this should be a bit longer in case the loopback is slow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No particular reason to keep it 5 sec. It should be fine to increase it (say, to 10s) as this init is a one time thing and it will help reduce flakiness for clients with slower callback functions. For clients with faster callbacks, this shouldn't be a problem as we are breaking out of the loop if callback succeeds early.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Increased to 10s
|


Overview
CONNECTwhen usingOAUTHBEARERwithSASL_SSLprotocol #1713 by delegating OAuth token refresh events to be handled by the background thread.Note that,
poll()will not trigger the OAuth refresh callbacks anymore.Problem
When providing
sasl.mechanismsasOAUTHBEARERand aoauth_cbin the conf, users were unable to make API calls likelist_topics(),create_topics()unlesspollwas called.(Snippet Source: #1713 (comment))
This happens because OAuth token refresh events get pushed to the main queue which is listened by poll(). These events are responsible for triggering the configured OAuth callback function and setting the token in the request. If
poll()is not called beforelist_topics(), the token would not be set in the request, causing the request to fail.Solution
Changes have been made to handle these refresh events in the background thread using the following librdkafka method calls:
rd_kafka_conf_enable_sasl_queue(conf, 1)- ensures all refresh events get pushed to the SASL queue instead of main queuerd_kafka_sasl_background_callbacks_enable(self->rk)- ensures all events in SASL queue are forwarded to the background queue.By using these 2 method calls, we have been able to ensure that OAuth callbacks get triggered from the background thread instead of
poll()Checklist